-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
process: refactor unhandled rejection handling #28228
Conversation
- Use constants instead of a dictionary and add comments about the behavior of each mode. - Use switch cases to handle the unhandled rejection modes. - Rename the run time value of the CLI option from `state` to `unhandledRejectionsMode`. - Return in the call site of `emitWarning` when `--unhandled-rejections=none` instead of inside the function.
Sadly, an error occurred when I tried to trigger a build. :( |
// --unhandled-rejection=strict: | ||
// Emit 'uncaughtException'. If it's not handled, print the error to stderr | ||
// and exit the process. | ||
// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this behavior (currently on master) looks weird to me. It contradicts with the description in test/parallel/test-promise-unhandled-error.js
:
// Verify that unhandled rejections always trigger uncaught exceptions instead
// of triggering unhandled rejections.
Even though that test specifically tests:
process.on('unhandledRejection', common.mustCall(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @BridgeAR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test description is faulty. The original implementation changed multiple times and it seems like the description was not updated appropriately.
The original intention was to replace the unhandled rejections with the exception. Later on it was decided that the current unhandled rejections hook behavior should not be influenced by the flag at all.
emitWarning(uid, reason); | ||
switch (unhandledRejectionsMode) { | ||
case kThrowUnhandledRejections: { | ||
fatalException(reason); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also funny enough there really isn't anything fatal about this function, or triggerFatalException
, as they give the user a chance to pretend nothing has happened by listening to uncaughtException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a legacy name. It does however reflect it's actual purpose. It is just unfortunate that we have an escape hatch for it with process.on('uncaughtException', fn)
.
// --unhandled-rejection=strict: | ||
// Emit 'uncaughtException'. If it's not handled, print the error to stderr | ||
// and exit the process. | ||
// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test description is faulty. The original implementation changed multiple times and it seems like the description was not updated appropriately.
The original intention was to replace the unhandled rejections with the exception. Later on it was decided that the current unhandled rejections hook behavior should not be influenced by the flag at all.
Landed in 370873c |
- Use constants instead of a dictionary and add comments about the behavior of each mode. - Use switch cases to handle the unhandled rejection modes. - Rename the run time value of the CLI option from `state` to `unhandledRejectionsMode`. - Return in the call site of `emitWarning` when `--unhandled-rejections=none` instead of inside the function. PR-URL: #28228 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
- Use constants instead of a dictionary and add comments about the behavior of each mode. - Use switch cases to handle the unhandled rejection modes. - Rename the run time value of the CLI option from `state` to `unhandledRejectionsMode`. - Return in the call site of `emitWarning` when `--unhandled-rejections=none` instead of inside the function. PR-URL: #28228 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
- Use constants instead of a dictionary and add comments about the behavior of each mode. - Use switch cases to handle the unhandled rejection modes. - Rename the run time value of the CLI option from `state` to `unhandledRejectionsMode`. - Return in the call site of `emitWarning` when `--unhandled-rejections=none` instead of inside the function. PR-URL: #28228 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
about the behavior of each mode.
state
to
unhandledRejectionsMode
.emitWarning
when--unhandled-rejections=none
instead of insidethe function.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes